Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project management updates #16

Merged
merged 7 commits into from
Jun 16, 2023
Merged

Project management updates #16

merged 7 commits into from
Jun 16, 2023

Conversation

khawkins
Copy link
Collaborator

@khawkins khawkins commented Jun 12, 2023

  • Switched to the webview presentation approach to prompting the user.
  • Implemented the process for opening an existing Starter Kit project.

There's still quite a bit of refactoring to go:

- Split out messaging to resource file(s)
- Testing, which will almost certainly require some refactoring to support it.

So it's a draft, but it's a working code base for the different project bootstrap use cases.

// It's actually important to run this async,
// because createNewProject() will not resolve
// its Promise until a path is selected,
// allowing the user to cancel the open dialog
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this proves out one of the things I like about staging our processes from a webview dialog. In this case, the user can cancel the open file dialog, or with the "open existing" use case, can select an invalid folder, and they can just try again without losing their state, by clicking the respective "launch" button in the webview again. I think that's a better way to go than anchoring with the controls that can so easily be dismissed from the UX.

@khawkins khawkins marked this pull request as ready for review June 15, 2023 23:21
@khawkins khawkins requested a review from a team as a code owner June 15, 2023 23:21
return new Promise(async (resolve) => {
this.projectConfigurationProcessor.getProjectManagementChoice(
(panel) => {
// It's actually important to run this async, because
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const folderUri =
await this.projectConfigurationProcessor.getProjectFolderPath();
if (!folderUri || folderUri.length === 0) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reject() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We explicitly don't want to reject. This is what allows for the "recoverable" case: the Promise will not be fulfilled one way or another, until a proper value is returned, allowing the user to cancel the folder picker any number of times and still continue the workflow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding comments to that effect in the relevant areas. It's a good place to add clarity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, awesome! That makes sense now. Thanks!

const folderUri =
await this.projectConfigurationProcessor.getProjectFolderPath();
if (!folderUri || folderUri.length === 0) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reject()?


// Is this the Offline Starter Kit repo?
try {
const oskInitialCommit =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put this in constants somewhere perhaps, or at least in a static readonly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

<body>
<p>Follow the prompts to configure the project.</p>
<p>
<strong>NOTE:</strong>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use tags here? ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tags?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugggh, I was making a joke about using blink tags, but git removed the <blink/>. Sorry!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha you know I had a feeling that's what you meant! But I was like, "But tags? ¯\(ツ)/¯ " We were on the same page all along.

projectDirStats = await stat(projectDir);
} catch (err) {
return reject(
`Project dir '${projectDir}' does not exist or is inaccessible.`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming these don't need to be l10n-ized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're just for tests, so I didn't bother.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp, I missed that it was in a test case. Thanks.

if (!projectDirStats.isDirectory()) {
return reject(`Project dir '${projectDir}' is not a directory.`);
}
return rm(projectDir, { recursive: true, force: true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is scary to me - is there any way this could be abused or broken and end up deleting someone's filesystem? Maybe we leave cleanup to end user?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, this is a test utility, so not a big deal. At first I thought it was part of the command. Ha. Still a bit scary to do a recursive delete on something unless it is absolutely necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that too, which is why I did all the stuff around validating the folder, as best I could. That said, they're just temp folders which will eventually get cleaned up anyway, so we could (ahem) remove the removal logic, if you all would prefer.

Copy link
Collaborator Author

@khawkins khawkins Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea to split the difference: make a data structure that only allows the created folder to be removed. That way we can virtually guarantee that we're talking about a temp folder. I'll code that up, shouldn't take a minute.

assert.equal(origCwd, process.cwd());
removeTempProjectDir(projectFolderUri.fsPath);
}
}).timeout(10000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if 10 seconds is long enough or if this can create flapper? Maybe 60secs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured I'd wait to see how it panned out in CI, but I'm totally open to upping it. Locally, it's just above the 2 sec default timeout, but you never know.

Copy link
Contributor

@dbreese dbreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. Some places where it is using return and not a reject() perhaps?

'git remote add origin https://github.com/salesforce/offline-app-developer-starter-kit.git'
);
await CommonUtils.executeCommandAsync(
'git fetch origin 99b1fa9377694beb7918580aab445a2e9981f611'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should SHA actually be synced using package.json so that the code and the test can share?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it an accessible property in the code here, and then just import it in the test. That's a good idea.

Copy link
Contributor

@sfdctaka sfdctaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@khawkins khawkins merged commit 63698ff into main Jun 16, 2023
@khawkins khawkins deleted the init_project branch June 16, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants